Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate "Add Account (advanced)" in WelcomeLogIn.swift #1246

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

lissine0
Copy link
Member

@tmolitor-stud-tu
Copy link
Member

I don't like this. the welcome login screen should be easy to use for non-techies and advanced options should be hidden in a separate screen for tech savy people to use.

@lissine0
Copy link
Member Author

the welcome login screen should be easy to use for non-techies and advanced options should be hidden in a separate screen for tech savy people to use.

That's exactly how it is in this PR. The screenshots above are what's shown when Add Account (advanced) is selected from the settings. Normal Add Account is still the same as before.

What this PR does, is basing Add Account (advanced) on Add Account instead of Account Edit
It feels more natural this way.

@tmolitor-stud-tu
Copy link
Member

Ah okay, that's okay. But, for the advanced view, could you remove the logo and change the explanatory text at the top to something explaining that this is an advanced expert account creation menu?

@tmolitor-stud-tu
Copy link
Member

We should make the direct tls switch only accessible (e.g. enabled) if the hostname or port fields contain something...

@lissine0
Copy link
Member Author

lissine0 commented Oct 6, 2024

Ah okay, that's okay. But, for the advanced view, could you remove the logo and change the explanatory text at the top to something explaining that this is an advanced expert account creation menu?

Which one of these looks do you prefer? this:

Or:



We should make the direct tls switch only accessible (e.g. enabled) if the hostname or port fields contain something...

Would you like to also switch the toggle to false if the user types something in hostname / port, then enables directTLS, then clears the hostname / port?

@tmolitor-stud-tu
Copy link
Member

I'd prefer the third one. @FriedrichAltheide @matthewrfennell what do you prefer?

Would you like to also switch the toggle to false if the user types something in hostname / port, then enables directTLS, then clears the hostname / port?

I'd say yes.

@matthewrfennell
Copy link
Member

I'd prefer the third one. @FriedrichAltheide @matthewrfennell what do you prefer?

I agree

@tmolitor-stud-tu
Copy link
Member

okay, third one it is :)

@lissine0 lissine0 force-pushed the add-account-advanced branch 3 times, most recently from 02cb0bf to 4be30fe Compare October 8, 2024 20:18
Copy link
Member

@tmolitor-stud-tu tmolitor-stud-tu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost mergeable :)

Monal/Classes/MLXMPPManager.m Outdated Show resolved Hide resolved
Monal/Classes/WelcomeLogIn.swift Outdated Show resolved Hide resolved
}
}

Toggle(isOn: $allowPlainAuth){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add a list of domains for which this toggle isn't allowed to be turned on? that list should currently only contain "conversations.im", but more might be added.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lissine0 this seems still to be missing, right?

}
}

Toggle(isOn: $allowPlainAuth){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lissine0 this seems still to be missing, right?

@lissine0
Copy link
Member Author

I made some other changes aside from those you asked for. Here's a demo of them:

Add_acconut_advanced.mp4

The reason why I did this is: the hardcoded port is only used in connection if a hardcoded server is set.
And I think the forceDirectTLS setting is also only used if there's a hardcoded server.
Thus I made the UI changes to make the users aware that those settings only take effect when setting a hardcoded server.

Also, in the old UI, 5222 is set by default as hardcoded port, even when doing a normal account adding. That can confuse users into thinking that StartTLS is used because of that value.
Thus I made 5222 the default only when a hardcoded server is set. Otherwise an empty string is used.
(We can store an empty string in a DB column of type Int because of a quirk in SQLite: the column's type is just a hint, and it doesn't prevent us from from storing a string in an int column)

And BTW, you were correct, the old add account (advanced) did indeed disallow plain auth for conversations.im
(XMPPEdit.m L342)

@lissine0 lissine0 force-pushed the add-account-advanced branch 2 times, most recently from 720ddcc to 2b86ae4 Compare October 17, 2024 07:47
NSArray* params = @[
server == nil ? @"" : server,
port == nil ? @"5222" : port,
nilDefault(server, @""),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that you are using the nilDefault macro :)

@tmolitor-stud-tu tmolitor-stud-tu merged commit 38cc560 into monal-im:develop Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants